New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add no-invalid-position-at-import-rule #5202
Add no-invalid-position-at-import-rule #5202
Conversation
lib/rules/no-invalid-position-at-import-rule/__tests__/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chuuddo Thank you for your first contribution. The pull request is looking good! It's great to see the naming conventions being followed too.
I've made some requests.
lib/rules/no-invalid-position-at-import-rule/__tests__/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes!
One last request, then it looks good to me.
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
I've left some trivial comments, but this PR is sufficient. 😄
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) { | ||
return; | ||
} | ||
|
||
if (node.type === 'atrule' && /^import$/i.test(node.name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[IMO] It seems a bit readable to me if using toLowerCase
. But this is just my opinion, so you can ignore it. 😃
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) { | |
return; | |
} | |
if (node.type === 'atrule' && /^import$/i.test(node.name)) { | |
const nodeName = node.name.toLowerCase(); | |
if (node.type === 'comment' || (node.type === 'atrule' && nodeName === 'charset')) { | |
return; | |
} | |
if (node.type === 'atrule' && nodeName === 'import') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion looks suitable. Just made some research and toLowerCase()
used much more often in rules.
@jeddy3 what do you think about it? Do we have any convention on this or should create one if not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought about toLowerCase
as @ybiquitous when I was first reviewing the pull request as we've used that approach in loads of places. However, the regex approach sort of grew on me because we use it for filtering at-rules and declarations when walking (like we did in the last rule we added), so I didn't mention it.
Do we have any convention on this or should create one if not?
I'm all for conventions, especially within rules, because they're such a time saver when reviewing code.
I'm not sure which of the approaches is more readable, though. So, let's go with @ybiquitous's toLowerCase
suggestion for now to be consistent with how we're treating names and values in other rules. We can then, if we want, refactor across the board later down the line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following on, we'll probably stick with toLowerCase
(for names and values) because (having looked myself) we often:
- do more string manipulation at the same time, e.g. trimming and slicing
- look for matches in arrays and sets
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Changelog entry added:
|
@jeddy3 just wondering, should this handle scss files too? Trying to figure out where I should report any issues https://github.com/twbs/stylelint-config-twbs-bootstrap/pull/110/checks?check_run_id=2447711414 |
@XhmikosR What SCSS code is triggering those errors? |
I think we can rejig the rule so that it ignores non-standard syntax (like dollar variables) while being true to the spec that the rule enforces:
@XhmikosR Can you create a bug ticket here in stylelint, please? |
Done, see #5263. Thanks :) |
@XhmikosR Fixed in |
Thanks! |
@jeddy3 should this also be fixed here?
Sorry for not catching this earlier, I just updated to the latest versions. |
Nope. That's the pattern the rule is warning against. I recommend moving that code into another file and importing it, e.g. Alternatively, if you don't want to align, you can use |
I agree with @jeddy3. Besides, it looks like out of this PR scope, but it seems good to me to migrate
|
@jeddy3 Thanks, I'll just ignore the warnings in the v4-dev branch 👍 @ybiquitous That's a different thing which we can't do for v4 because it's a breaking change. |
Closes #5133.
No, it's self-explanatory.